-
Notifications
You must be signed in to change notification settings - Fork 724
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Parse decimals and output for rational fields in protocol parameters and genesis #2992
Parse decimals and output for rational fields in protocol parameters and genesis #2992
Conversation
023a7e7
to
b10f7c2
Compare
Output before compared to output after:
|
Alonzo genesis before and after:
|
b4dc382
to
34b901c
Compare
34b901c
to
e641457
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we only need the ToJSON
side of things, and the FromJSON
side should already work. See details below. Can you check?
toRationalJSON :: Rational -> Aeson.Value | ||
toRationalJSON r = | ||
case Scientific.fromRationalRepetend (Just 5) r of | ||
Right (s, Nothing) -> toJSON s | ||
_ -> toJSON r |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. For whatever reason, although the Rational
instance for FromJSON
accepts both number and object style, the ToJSON
instance never produces numbers, only object style:
instance (ToJSON a, Integral a) => ToJSON (Ratio a) where
toJSON r = object [ "numerator" .= numerator r
, "denominator" .= denominator r
]
{-# INLINE toJSON #-}
toEncoding r = E.pairs $
"numerator" .= numerator r <>
"denominator" .= denominator r
{-# INLINE toEncoding #-}
So we do need this toRationalJSON
workaround.
Perhaps that is worth proposing upstream as a PR to do the above trick for the instance provided by the library.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And also:
λ> encode (1 :: Rational)
"{\"numerator\":1,\"denominator\":1}"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Until and if such change gets merged upstream, I'd also suggest importing toRationalJSON
from Cardano.Api.Orphans
instead of redefining another copy again here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hesitate mainly because I can't use it unless I export it from Cardano.Api
anyway, in which case it becomes part of our API which I don't want especially because we want to push it upstream in a way that doesn't need an extra function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not realize Cardano.Api.Orphans
is an exposed module. That is a very strange thing to do considering that anyone who imports Cardano.Api
will get all those orphan instances into scope anyways (all instances are imported through modules transitively). I would suggest moving that module into other-modules
in the cabal file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, my mistake. I thought I was using toRationalJSON
from outside cardano-api
, but I don't. But it still is a bit strange to export functions from a module that called Orphans. Not a hard objection though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will move it to a new module Cardano.Api.Json
. Seems the cleanest.
e641457
to
be40af9
Compare
be40af9
to
56485ff
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left a few suggestions, but none of them are problematic. This PR is strictly better than it was before, in fact I very recently thought about suggesting this exact same approach for formatting Rational in Aeson, but haven't got around doing it. I am just not sure what to suggest as a default limit for repetend detection
56485ff
to
d07abec
Compare
d07abec
to
6c978ec
Compare
bors merge |
Build succeeded: |
No description provided.